Skip to content

fix: support py_cc_toolchain#1131

Closed
jbedard wants to merge 1 commit into
mainfrom
1095-inc-macos
Closed

fix: support py_cc_toolchain#1131
jbedard wants to merge 1 commit into
mainfrom
1095-inc-macos

Conversation

@jbedard

@jbedard jbedard commented Jun 19, 2026

Copy link
Copy Markdown
Member

Changes are visible to end-users: yes/no

  • Searched for relevant documentation and updated as needed: yes/no
  • Breaking change (forces users to change their own code or config): yes/no
  • Suggested release notes appear below: yes/no

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

@aspect-workflows

aspect-workflows Bot commented Jun 19, 2026

Copy link
Copy Markdown

✨ Aspect Workflows Tasks

📅 Fri Jun 19 20:28:35 UTC 2026

❌ 2 failed tasks

  • ❌ test (test-root-bazel-8) · ⏱ 2m 2s · 🐙 GitHub Actions · ☑️ Check
    💬 failed in test · Bazel test failed (1 test failed)
  • ❌ test (test-root-bazel-9) · ⏱ 2m 51s · 🐙 GitHub Actions · ☑️ Check
    💬 failed in test · Bazel test failed (1 test failed)

✅ 5 successful tasks

  • ✅ buildifier · ⏱ 22.2s · 🐙 GitHub Actions · ☑️ Check
    💬 Format complete (clean)
  • ✅ gazelle · ⏱ 27.4s · 🐙 GitHub Actions · ☑️ Check
    💬 Gazelle complete (clean)
  • ✅ test (test-e2e-bazel-8) · ⏱ 2m 20s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (150/150 passed)
  • ✅ test (test-e2e-bazel-9) · ⏱ 3m 7s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (145/145 passed · 128 cached)
  • ✅ test (test-examples-uv_pip_compile-bazel-8) · ⏱ 32.2s · 🐙 GitHub Actions · ☑️ Check
    💬 Bazel test complete (1/1 passed · 1 cached)

🔁 Reproduce

❌ test (test-root-bazel-8 · test-root-bazel-9)

aspect test -- //uv/private/uv_hub:snapshots_9_test
# vanilla bazel
bazel test -- //uv/private/uv_hub:snapshots_9_test

Install aspect: docs.aspect.build/cli/install


⏱ Last updated Fri Jun 19 20:32:29 UTC 2026 · 📊 GitHub API quota 23/15,000 (0% used, resets in 56m)
🚀 Powered by Aspect CLI (v2026.24.11)  |  Aspect Build · X · LinkedIn · YouTube

@github-actions

Copy link
Copy Markdown

py_binary startup benchmark

Version Mean (ms) Median (ms) ± stddev vs BCR vs main Build (s)
BCR 1.11.5 (baseline) 294.828 293.121 ±6.915 40.22
HEAD main 57.239 57.105 ±0.858 -80.6% 12.53
This PR 57.223 56.867 ±1.466 -80.6% -0.0% 11.25

Measured with hyperfine --warmup 5 --runs 50 on Linux
Gate: PR vs HEAD main (threshold: 10%). BCR is shown only as a historical baseline.
Build time: cold bazel build //:bench with isolated output base, no disk cache.

sys.path quality

Version sys.path entries distinct site-packages roots duplicate realpaths
BCR 1.11.5 (baseline) 6 1 0
HEAD main 7 2 0
This PR 7 2 0

sys.path quality measured by bench_syspath inside the assembled venv. Duplicate realpaths indicate symlink redundancy; many distinct site-packages roots suggest an inefficient venv layout.

@tamird

tamird commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

My clanker exercised this path through a rules_py 2 integration and
found that a compile-only test can silently resolve rules_python's
fallback C toolchain instead of the generated PBS one.

The replacement commit is
tamird@251eb97. It compares the
selected runtime and C-toolchain implementation directly, covers regular
and free-threaded selection, keeps the Windows full and stable import
libraries disjoint, and imports an extension whose PY_VERSION_HEX
must match sys.hexversion. Free-threaded toolchains omit
headers_abi3 because CPython rejects Py_LIMITED_API there.

That commit is currently stacked on several open prerequisite changes, so
I have not opened a competing PR. Are you okay with me superseding this
draft once those land, or would you prefer to fold the commit into this
branch?

@jbedard

jbedard commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

I'm not attached to this branch at all if you have an alternate. Your link is a 404 though...

@tamird

tamird commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The reviewed replacement is now
tamird@4855688.
It verifies exact platform, runtime, and C-payload selection through public
provider data, exercises regular and free-threaded compile/import paths, and
does not advertise the stable ABI for free-threaded runtimes.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Downstream data point: this fixes a real nanobind_extension on Linux — not just the <Python.h> smoke test.

We hit #1095 in a monorepo that sources Python toolchains entirely from rules_py's python_interpreters (PBS) extension, with no direct rules_python dep, so nanobind's @rules_python//python/cc:current_py_cc_headers had no registered py_cc_toolchain.

Tested this branch (58fc7e46) via git_override on Linux x86_64 with a Zig hermetic cc toolchain, against a real nanobind_extension target:

  • Analysis resolves cleanly — the prior No matching toolchains found for @@rules_python+//python/cc:toolchain_type is gone.
  • The extension compiles + links against the PBS interpreter's headers and imports at runtime, passing under CPython 3.11, 3.12, and 3.13.

Because this drives the real current_py_cc_headers path (compile → link → import), it shouldn't be satisfiable by rules_python's fallback C toolchain — which I believe is the masking risk @tamird raised about the bare <Python.h> compile in the PR's e2e case. If it'd help, I'm happy to contribute this nanobind case as an e2e fixture (nanobind_extension → import smoke test, ~15 lines) so there's coverage that exercises the generated PBS toolchain end-to-end rather than a bare header compile.

Separately (not a claim about this PR): with #1095 previously blocking analysis, we'd never reached runtime to observe #1048 on this path — and at this commit we see no sign of the sys.base_prefix issue on any of 3.11/3.12/3.13.

This (or @tamird's alternative) reaching a mergeable state is the last blocker for our rules_py v2 adoption on native-extension packages, so glad to help validate — happy to re-run against 4855688 once its prerequisites land.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Correction + a sharper result after digging into toolchain resolution.

My earlier "shouldn't be satisfiable by rules_python's fallback C toolchain" was too strong — the masking is real, but version-dependent, and I can now show exactly where it does and doesn't bite.

Setup: python_interpreters as the rules_python drop-in; rules_python present only transitively (resolves to 1.7.0 here), its default pythons_hub registering py_cc toolchains for 3.11 only in this graph. Building a real nanobind_extension, per target Python version:

Target Py Without this PR With this PR
3.13 ❌ analysis fails: No matching toolchains found for @@rules_python+//python/cc:toolchain_type (rules_python's hub has no 3.13 py_cc) ✅ resolves @@aspect_rules_py++python_interpreters+…//:py_cc_toolchain
3.11 ⚠️ builds, but masked — resolves @@rules_python++python+python_3_11_…//:py_cc_toolchain, i.e. rules_python's own 3.11 headers, not the configured PBS interpreter ✅ resolves the PBS python_interpreters toolchain

So the NOTE in the e2e case is right that a real native build can be masked when rules_python is present — but only for the Python version(s) its transitive hub happens to register (3.11 here). For any other version (3.13) the gap is unmasked and the build genuinely fails without the registration. And even in the masked case the fix matters: without it, @rules_python//python/cc:current_py_cc_headers silently resolves rules_python's own interpreter headers rather than the PBS interpreter you configured — a latent header/runtime-interpreter mismatch (the concern @tamird raised, made concrete). With the PR, resolution correctly points at the python_interpreters toolchain across 3.11/3.12/3.13. All verified via --toolchain_resolution_debug.

Happy to turn this into the repro PR mentioned above: a real nanobind_extension e2e case that (a) fails on 3.13 without the registration and (b) asserts the selected py_cc toolchain is the python_interpreters one rather than rules_python's — so the masked-but-wrong 3.11 case is caught too. Let me know if you'd like it against e2e/cases/.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

One clarification for anyone finding this thread later: my testing above was against this branch (58fc7e46), which is now ~117 commits behind main. Current main already registers a py_cc_toolchain in python_interpreters (py/private/interpreter/repository.bzl) and ships a passing e2e/cases/pbs-cc-toolchain case that compiles a real extension against @rules_python//python/cc:current_py_cc_headers / current_py_cc_libs and asserts the exact PBS interpreter (plus abi3 / no-libpython-on-macOS) — so #1095 looks resolved on main via that route, not this branch.

Leaving my findings above for reference, but for anyone hitting the py_cc gap: point at main. I'm separately checking whether the nanobind-specific path needs any additional coverage on current main and will follow up if so.

@ctcjab

ctcjab commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the above: opened #1299 with the nanobind e2e case — a real nanobind_extension (a build_test plus a runtime import/call under a PBS 3.13 runtime) alongside pbs-cc-toolchain, covering the @rules_python//python/cc:current_py_cc_headers path the way most native extensions actually reach it. Confirms the py_cc registration on main works for the nanobind consumer and guards it against regression.

@jbedard

jbedard commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

I think #1187 (and maybe #1202?) accomplished everything this PR was trying to do, and the remaining tests here seem to overlap with existing tests.

@ctcjab the test you added in #1299 still shows reproduces a bug that needs fixing, I'm not sure if @tamird is aware of that issue.

@jbedard jbedard closed this Jul 18, 2026
@jbedard
jbedard deleted the 1095-inc-macos branch July 18, 2026 19:26

tamird commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@jbedard which bug do you mean? #1299's Bazel 8/9 e2e checks ran both nanobind targets successfully. Its deps edge is intentional: version-sensitive extensions must retain the terminal Python configuration, while data resets to the inherited configuration. The explicit imports = ["."] is also intentional under rules_py's -I launcher so the package-local .so has a declared import root. Current pbs-cc-toolchain and cc-deps coverage already verify that the selected runtime and C payload match, including PY_VERSION_HEX == sys.hexversion. Is there another failing configuration or invariant you mean?

@jbedard

jbedard commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

@tamird oh, you are correct, it looks like your CC changes also fixed the test in #1299.

@ctcjab can you confirm the latest HEAD fixes your issue?

jbedard pushed a commit that referenced this pull request Jul 18, 2026
…1299)

Adds an e2e case that builds a real `nanobind_extension` in a graph
whose only Python toolchains come from `python_interpreters` (the
rules_python `python.toolchain()` drop-in), exercising the
`@rules_python//python/cc:current_py_cc_headers` path that nanobind and
pybind11 use to resolve Python headers.

`pbs-cc-toolchain` already covers the bare `cc_binary` +
`current_py_cc_headers`/`current_py_cc_libs` path; this adds the
**nanobind** consumer, which is how most real-world native extensions
reach `current_py_cc_headers`. It guards the py_cc toolchain
registration (issue #1095) against regression.

Two targets:
- `example_ext_build_test` — a `build_test` over the
`nanobind_extension`; fails at analysis if `python_interpreters` does
not register `@rules_python//python/cc:toolchain_type`.
- `example_ext_test` — compiles, links, loads and calls the extension
under a PBS runtime (py 3.13).

**End-user visible:** no (test-only). **Breaking:** no.

**Test plan:** `bazel test //nanobind-py-cc/...` in `e2e/cases` passes
locally (build_test + runtime import/call; Linux x86_64, llvm
toolchain).

Context: came out of testing the py_cc gap for a real nanobind consumer
(discussed in #1131); @jbedard suggested opening this.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants